Replace provision Tagging tree by component #5550

PanSpagetka · 2019-05-09T10:33:08Z

Replace provision Tagging tree with a new component

Links [Optional]

#5515

Screenshot

Steps for Testing/QA [Optional]

Provision VM/Instance -> Purpose tab

PanSpagetka · 2019-05-09T10:33:22Z

@miq-bot add_label WIP

PanSpagetka · 2019-05-22T13:41:17Z

@miq-bot add_label tagging

PanSpagetka · 2019-06-10T09:40:48Z

@miq-bot remove_label wip

skateman

You have a few antipatterns, please address them.

skateman · 2019-06-10T10:28:00Z

app/javascript/miq-redux/middleware.js

-    const params = {id: store.getState().tagging.appState.affectedItems[0], cat: tag.tagCategory.id, val: tag.tagValue.id, check: 1, tree_typ: 'tags' };
+    const ids = [...store.getState().tagging.appState.assignedTags.map( t => t.values.map(val => val.id)), tag.tagValue.id].flat();
+    console.log(ids);
+    const params = (meta.type === 'provision'


This is a 🦛 step backwards 🦛 with the ternary operator... 🔴 🏴 @himdel @martinpovolny @Hyperkid123 any idea how else we could do this?

What do you mean by step back?

There are not many options with expression like this. You can do classic if statement. Or switch or maybe invoke a function like this:

// or extract it somewhere as a helper function params = ((type, state) => ({ provision: state.something..., default: 'b' })[type || 'default'])(meta.type, state)

My problem here is the special case when the tagging is on the provisioning screen. Anything like this has the potential to grow into a huge case with a ton of exceptions

Ah ok, i would suggest then to add a Prop to components that are using this and create the params inside the component and send it as a (meta)parameter to the action. We have the thunk middleware so we can get to the current state inside action creators almost exactly the same way as in middleware.

There should be a convention for this.

Every form not just talking to the API will have the same problem - the definiton of what to send via http can not be part of the component itself.

Passing a function to transform state to request data sounds reasonable, but maybe we should really do it the same way in all the react forms.

skateman · 2019-06-10T10:28:08Z

app/javascript/miq-redux/middleware.js

@@ -5,7 +5,12 @@ import promiseMiddleware from 'redux-promise-middleware';
 export const taggingMiddleware = store => next => action => {
  const { type, meta, tagCategory, tag } = action;
  if (meta && meta.url) {
-    const params = {id: store.getState().tagging.appState.affectedItems[0], cat: tag.tagCategory.id, val: tag.tagValue.id, check: 1, tree_typ: 'tags' };
+    const ids = [...store.getState().tagging.appState.assignedTags.map( t => t.values.map(val => val.id)), tag.tagValue.id].flat();
+    console.log(ids);


Probably not wanted

skateman · 2019-06-10T10:29:09Z

app/controllers/application_controller/miq_request_methods.rb

+    end
+
+    assignments = Classification.select { |tag| vm_tags.include?(tag.id) }
+    assigned_tags = Classification.select { |tag| vm_tags.include?(tag.id) }.map do |tag|


Why are you duplicating code here?

skateman · 2019-06-10T10:29:50Z

app/controllers/application_controller/miq_request_methods.rb

-        @ci_node[:children] = @ci_kids if @ci_kids.present?
-        all_tags.push(@ci_node) if @ci_kids.present?
+    # for some reason @tags is set in wf, and it is changed by map bellow which causes bugs
+    wf.instance_variable_set(:@tags, nil)


What's wf? If it is undocumented behavior, please look it up and document it.

stands for workflow, used a lot in automate code (309 occurences)

skateman · 2019-06-10T10:30:15Z

app/controllers/application_controller/miq_request_methods.rb

+        :save_url   => '',
+        :cancel_url => ''
+      }
+    @tags1 = {:tags => tags, :assignedTags => assigned_tags, :affectedItems => []}


Why @tags1? Why not 42?

skateman · 2019-06-10T10:31:22Z

app/controllers/application_controller/miq_request_methods.rb

    end
-    @all_tags_tree = TreeBuilder.convert_bs_tree(all_tags).to_json # Add ci node array to root of tree
+    tags = tags.map { |t| {:id => t[:name], :singleValue => t[:single_value] }.merge(t) }


You have better hash manipulation techniques for this, check out the documentation

skateman · 2019-06-10T10:32:22Z

app/controllers/application_controller/miq_request_methods.rb

    end
-    @all_tags_tree = TreeBuilder.convert_bs_tree(all_tags).to_json # Add ci node array to root of tree
+    tags = tags.map { |t| {:id => t[:name], :singleValue => t[:single_value] }.merge(t) }
+    tags.map do |t|


I'm not very happy about the map here, maybe each. Or map! and do the delete properly.

himdel · 2019-06-11T13:07:36Z

app/views/miq_request/_prov_field.html.haml

-                   :locals => {:tree_id       => "all_tags_treebox",
-                               :tree_name     => "all_tags_tree",
-                               :bs_tree       => @all_tags_tree,
-                               :oncheck       => "miqOnCheckProvTags",


also remove the miqOnCheckProvTags function please (and from the whitelist)?

(If it was used miq_prov_vars would still need the split .. but it is dead now :).)

himdel · 2019-06-11T13:13:00Z

app/controllers/application_controller/miq_request_methods.rb

+    # for some reason @tags is set in wf, and it is changed by map bellow which causes bugs
+    wf.instance_variable_set(:@tags, nil)
+    tags = wf.allowed_tags
+    tags.each do |cat|


Something like

tags = wf.allowed_tags.map do |cat| { :values => cat[:children].map do |tag| {:id => tag.first, :description => tag.second[:description]} end, :id => cat[:name], :singleValue => cat[:single_value], } end

would probably be better than overwriting items in the allowed_tags array?

In general please prefer functional style over each :)

Thanks, I was stuck with an idea, that I need to modify existing hash....while create a new is much nicer

himdel · 2019-06-11T13:14:21Z

app/controllers/application_controller/miq_request_methods.rb

+    end.uniq
+
+    @button_urls = {
+        :save_url   => '',


This looks suspicious.. you're never saving or cancelling?

(Maybe just a comment why?)

It shouldn't be there...

himdel · 2019-06-11T13:28:04Z

app/javascript/miq-redux/middleware.js

@@ -5,10 +5,11 @@ import promiseMiddleware from 'redux-promise-middleware';
 export const taggingMiddleware = store => next => action => {
  const { type, meta, tagCategory, tag } = action;
  if (meta && meta.url) {


switch (type) { case 'UI-COMPONENTS_TAGGING_TOGGLE_TAG_VALUE_CHANGE': $.post(meta.url, meta.params(meta.type, store.getState(), tag)); break; case 'UI-COMPONENTS_TAGGING_DELETE_ASSIGNED_TAG': $.post(meta.url, meta.onDelete(meta.type, params, tag.tagValue.id)); break; }

and move that check: 0 to onDelete definition?

~~Or ideally call the same method, since you're passing the type anyway.~~ (type != meta.type, sorry)

The point is, there should be only 1 place dealing with this logic, not 2, or 3.

(maybe onDelete should not take computed params, and do it by itself, from state, that way maybe you can also unify the args)

skateman · 2019-06-11T20:21:55Z

@PanSpagetka please avoid using commit messages that lack context, having the "code styling" commit in the master's git log will be totally useless for the future 😕

martinpovolny · 2019-07-30T10:25:35Z

ping @terezanovotna

himdel · 2019-07-30T12:19:20Z

app/views/miq_request/_prov_field.html.haml

@@ -290,26 +289,20 @@
            = field_hash[:notes]
      - elsif [:tag_ids, :vm_tags].include?(field)
        -# tree control for tags fields
-        .col-md-8
+        .col-md-10
          #all_tags_treebox.treeview-pf-hover.treeview-pf-select{:style => "color:#000; overflow: hidden;"}


We should probably remove these tree classes and style overrides since this is no longer a tree.

Also, the div is actually empty, and the code setting check is not used anymore, so you can just remove lines 293 to 299

himdel · 2019-07-30T12:20:42Z

3 more remnants of the tree:

app/views/miq_request/_prov_field.html.haml
293:          #all_tags_treebox.treeview-pf-hover.treeview-pf-select{:style => "color:#000; overflow: hidden;"}

app/views/miq_request/_prov_edit.html.haml
8:  miqDeleteTreeCookies('all_tags_tree')

app/controllers/vm_common.rb
1196:        presenter[:clear_tree_cookies] = "all_tags_tree"

himdel · 2019-07-30T12:39:09Z

Also looks like this will still need a react-ui-components update:

Warning: Failed prop type: Invalid prop `tags.tags[0].id` of type `string` supplied to `TaggingWrapper`, expected `number`.

Warning: Failed prop type: Invalid prop `tags[0].id` of type `string` supplied to `Tagging`, expected `number`.

Warning: Failed prop type: Invalid prop `tagCategories[0].id` of type `string` supplied to `CategoryModifier`, expected `number`.

Warning: Failed prop type: Invalid prop `tagCategories[0].id` of type `string` supplied to `TagSelector`, expected `number`.

(I don't think the component should ever expect number for id)

himdel · 2019-07-30T12:51:05Z

It's currently impossible to remove the last tag:

(it just reappears after the server responds)

himdel · 2019-07-30T12:54:46Z

When changing a tag from one of the "only a single value" categories:

chose Category > Foo
the UI shows Category: Foo on the right, sends

id: new
ids_checked[]: 86
tree_typ: tags
check: 0

change to Category > Bar

the UI shows Category: Bar on the right, sends

id: new
ids_checked[]: 86
ids_checked[]: 87
tree_typ: tags

Note that the first tag id remained in the second request, so we're sending 2 tags from a "single tag only" category.

EDIT: this works in conjunction with the backend discarding all but the last id in each single-value category .. and the last chosen will be last simply because it's added to the list last .. and the client state gets updated by the server state on every change. I think this works :)

himdel · 2019-08-01T10:33:36Z

(
Still 2 bugs:

sending ids_checked[]: 87 in one request and then ids_checked[]: 87 ; ids_checked[]: 86 in the other (both in same cateogry), 87 remains checked
sending ids_checked[]: 87 ; ids_checked[]: 86 in one request and then deleted: 86 will set 87 (instead of none), after that, deleted: 87 goes back to showing 86 (that's because build_tags_tree doesn't mutate @edit[:new][:vm_tags] but only a local copy, so [87,86] remain there when deleting

)

himdel · 2019-08-01T11:45:03Z

app/controllers/application_controller/miq_request_methods.rb

@@ -1013,7 +1013,7 @@ def build_tags_tree(wf, vm_tags, edit_mode)
        end
      }
    end.uniq
-
+    # assigned_tags.map! { |c| c[:values].delete_if {|v| v[:id] == params[:deleted].to_i} }


Probably want to delete this?

himdel · 2019-08-01T11:45:39Z

app/javascript/components/taggingWrapper.jsx

@@ -21,7 +21,7 @@ const params = (type = 'default', state, tag = {}) => ({
 })[type]

 const onDelete = (type = 'default', params = [], deleted_element) => ({
-  provision: {...params, check: 0, ids_checked: params.ids_checked.filter(element => element !== deleted_element)},
+  provision: {...params, check: 0, ids_checked: params.ids_checked.filter(element => element !== deleted_element), deleted: deleted_element },


I guess sending deleted no longer matters?

himdel · 2019-08-01T11:46:22Z

The spec failures look relevant, the post is called with different params now so the test is wrong.

miq-bot · 2019-08-05T08:26:51Z

Checked commits PanSpagetka/manageiq-ui-classic@f7abada~...2ce52a3 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

himdel · 2019-08-05T11:46:36Z

LGTM, merging :)

@PanSpagetka One thing that needs fixing - as a separate PR - we should never be using numeric ids in javascript.

Due to the limitation of MAX_SAFE_INTEGER in JS (see #1405 for details), we need to consistently treat any database id as a string in JS.

Can you change that plase?

martinpovolny · 2019-08-05T12:48:25Z

Great!

Are there any further places where we need the new tagging component?

skateman · 2019-08-05T13:06:45Z

Yes, two of them, @PanSpagetka already working on them.

miq-bot added the wip label May 9, 2019

PanSpagetka changed the title ~~[WIP] Replace provision Tagging try by component~~ [WIP] Replace provision Tagging tree by component May 9, 2019

PanSpagetka force-pushed the provision-tagging-replace branch 3 times, most recently from b0f1698 to 3f7100c Compare May 22, 2019 13:40

miq-bot added the tagging label May 22, 2019

PanSpagetka force-pushed the provision-tagging-replace branch from 3f7100c to 4e71039 Compare June 10, 2019 09:35

miq-bot changed the title ~~[WIP] Replace provision Tagging tree by component~~ Replace provision Tagging tree by component Jun 10, 2019

miq-bot removed the wip label Jun 10, 2019

skateman suggested changes Jun 10, 2019

View reviewed changes

PanSpagetka force-pushed the provision-tagging-replace branch 5 times, most recently from 380c263 to e198f1d Compare June 11, 2019 11:11

himdel reviewed Jun 11, 2019

View reviewed changes

miq-bot added the unmergeable label Jun 14, 2019

lina-is-here mentioned this pull request Jun 18, 2019

[1LP][RFR] fixed CloudProviderInstancesView and tags ManageIQ/integration_tests#8942

Merged

PanSpagetka force-pushed the provision-tagging-replace branch from 052379f to af03282 Compare July 24, 2019 08:07

miq-bot removed the unmergeable label Jul 24, 2019

PanSpagetka closed this Jul 24, 2019

PanSpagetka reopened this Jul 24, 2019

martinpovolny added the ivanchuk/no label Jul 24, 2019

martinpovolny added react trees labels Jul 30, 2019

Hyperkid123 approved these changes Jul 30, 2019

View reviewed changes

himdel reviewed Jul 30, 2019

View reviewed changes

PanSpagetka force-pushed the provision-tagging-replace branch from 4f868a0 to 4b2a2f8 Compare July 31, 2019 09:38

PanSpagetka added 3 commits July 31, 2019 15:05

Use new version of react-ui-components

a410aac

Change tags PropType to string or number

c9528dc

Remove all tags tree

bf430f2

PanSpagetka force-pushed the provision-tagging-replace branch from 4b2a2f8 to b467f54 Compare July 31, 2019 13:05

Fix delete for provision tagging

1bd910a

PanSpagetka force-pushed the provision-tagging-replace branch from b467f54 to 1bd910a Compare August 1, 2019 11:00

himdel reviewed Aug 1, 2019

View reviewed changes

Fix tagging middleware specs

2ce52a3

himdel merged commit 42718a6 into ManageIQ:master Aug 5, 2019

himdel self-assigned this Aug 5, 2019

himdel added this to the Sprint 117 Ending Aug 5, 2019 milestone Aug 5, 2019

h-kataria mentioned this pull request Feb 21, 2020

Replace tagging tree with a React component #5515

Closed

Replace provision Tagging tree by component #5550

Replace provision Tagging tree by component #5550

Conversation

PanSpagetka commented May 9, 2019 • edited Loading

Links [Optional]

Screenshot

Steps for Testing/QA [Optional]

PanSpagetka commented May 9, 2019

PanSpagetka commented May 22, 2019

PanSpagetka commented Jun 10, 2019

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

himdel Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

skateman commented Jun 11, 2019

martinpovolny commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Jul 30, 2019

himdel commented Jul 30, 2019 • edited Loading

himdel commented Jul 30, 2019 • edited Loading

himdel commented Jul 30, 2019 • edited Loading

himdel commented Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Aug 1, 2019

miq-bot commented Aug 5, 2019

himdel commented Aug 5, 2019

martinpovolny commented Aug 5, 2019

skateman commented Aug 5, 2019

PanSpagetka commented May 9, 2019 •

edited

Loading

Hyperkid123 Jun 10, 2019 •

edited

Loading

Hyperkid123 Jun 10, 2019 •

edited

Loading

himdel Jun 11, 2019 •

edited

Loading

himdel Jun 11, 2019 •

edited

Loading

himdel commented Jul 30, 2019 •

edited

Loading

himdel commented Jul 30, 2019 •

edited

Loading

himdel commented Jul 30, 2019 •

edited

Loading

himdel commented Aug 1, 2019 •

edited

Loading